Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Server error 500 creating a PR against a branch with changes to a file that has been renamed in the target branch #22083

Closed
parnic opened this issue Dec 9, 2022 · 2 comments · Fixed by #22118
Closed

Server error 500 creating a PR against a branch with changes to a file that has been renamed in the target branch #22083

parnic opened this issue Dec 9, 2022 · 2 comments · Fixed by #22118
Labels
type/bug
Milestone
1.17.4

Comments

@parnic
Copy link
Contributor

parnic commented Dec 9, 2022 •
edited
Loading

Description

Creating a PR against a branch with changes to a file that has since been renamed in the target branch will cause an error 500 and inability to open the PR. When this happens, the server logs only say

2022/12/09 14:35:19 ...ers/web/repo/pull.go:1244:CompareAndPullRequestPost() [E] NewPullRequest: git apply --check: exit status 1

so it's difficult to understand what to do to work around it. The appropriate workaround is to update the source branch with latest from the target branch before opening the PR.

Specific repro steps:

  1. Create a repo.
    1. https://try.gitea.io/parnic-sks/pr-renamed-files-test
  2. Create a file. Make sure it's long enough that a rename + changing contents will still detect the rename.
    1. https://try.gitea.io/parnic-sks/pr-renamed-files-test/commit/aebe80c2ae9dd290c8359cb4e6c4df45ed23fc19
  3. Create a branch from this commit.
    1. https://try.gitea.io/parnic-sks/pr-renamed-files-test/src/branch/file-change
  4. Change something in that file and branch.
    1. https://try.gitea.io/parnic-sks/pr-renamed-files-test/commit/0bb985908ca24a8fd06972ee8a38a28297957ed8
  5. Back in the target branch, rename the file and make a small change in it as well.
    1. https://try.gitea.io/parnic-sks/pr-renamed-files-test/commit/323c74cae5fda0b276daad71df0905503af7d331
  6. Attempt to open a PR from the outdated source branch to the target branch with the renamed + changed file.
    1. https://try.gitea.io/parnic-sks/pr-renamed-files-test/compare/main...file-change
  7. The diff on this page looks fine, but trying to actually create the PR results in a 500 error on client and "git apply --check" error on server.

I can supply specific git commands needed for each step if it's helpful, but hopefully they're obvious from the given links.

Locally running the git apply command that the server is presumably running results in the following message (that doesn't show up on the server):

> git show 0bb985908ca24a8fd06972ee8a38a28297957ed8 | git apply --check
error: 1.txt: No such file or directory

which is what clued me into the problem.

Gitea Version

1.17.3, can also repro on try.gitea.io at v1.19.0+dev-171-g2779d47ad

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

See description

Screenshots

image

image

Git Version

2.37.3 on the Gitea v1.17.3 instance, whatever version is on try.gitea.io

Operating System

Ubuntu 20.04.5 aarch64

How are you running Gitea?

Built myself with a few modifications from v1.17.3 (mostly cherry-picking things from the future).

https://try.gitea.io however that's built

Database

PostgreSQL
@parnic parnic added the type/bug label Dec 9, 2022
@lunny lunny added this to the 1.17.4 milestone Dec 9, 2022
@zeripath
Copy link
Contributor

OK, that's a new error from git apply.

I'm actually not sure if we should keep the git apply path around anymore - I think the git read-tree path might be covering all of our bases and I only kept the old path around because it was new.

So, I reckon we should:

  1. use the provided reproducer information to create a test-case.
  2. detect the error as above - which would be simple and backport that.
  3. Disable the git-apply route but add a config value to allow fallback - which can be removed in Gitea 1.20

zeripath added a commit to zeripath/gitea that referenced this issue Dec 13, 2022
@zeripath
Correctly handle moved files in apply patch
494a122 
Moved files in a patch will result in git apply returning:

```
error: {filename}: No such file or directory
```

This wasn't handled by the git apply patch code. This PR adds handling
for this.

Fix go-gitea#22083

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this issue Dec 13, 2022
Merged
@zeripath
Copy link
Contributor

I've put up a PR to just add the handling for "No such file or directory", I'll look at creating the testcase this evening.

zeripath added a commit to zeripath/gitea that referenced this issue Dec 14, 2022
@zeripath
Add setting to disable the git apply step in test patch
0feb4dc 
For a long time Gitea has tested PR patches using a git apply --check
method and in fact prior to the introduction of a read-tree assisted
three-way merge in go-gitea#18004, this was the only way of checking patches.

Since go-gitea#18004, the git apply --check method has been a fallback method,
only used when the read-tree method has detected a conflict. The
read-tree assisted three-way merge method is much faster and less
resource intensive method of detecting conflicts. go-gitea#18004 kept the git
apply method around because it was thought possible that this fallback
might be able to rectify conflicts that the read-tree three-way merge
detected. I am not certain if this could ever be the case.

Given the uncertainty here and the now relative stability of the
read-tree method - this PR makes using this fallback optional and
disables it by default. The hope is that users will not mention any
significant difference in conflict detection and we will be able to
remove the git apply fallback in future, and/or improve the read-tree
three-way merge method to catch any conflicts that git apply method
might have been incorrectly detecting.

(See https://github.com/go-gitea/gitea/issues/22083\#issuecomment-1347961737)

Ref go-gitea#22083

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this issue Dec 14, 2022
Merged
lunny pushed a commit that referenced this issue Dec 14, 2022
@zeripath @techknowlogick
Correctly handle moved files in apply patch (#22118)
cdc4345 
Moved files in a patch will result in git apply returning:

```
error: {filename}: No such file or directory
```

This wasn't handled by the git apply patch code. This PR adds handling
for this.

Fix #22083

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
zeripath added a commit to zeripath/gitea that referenced this issue Dec 14, 2022
@zeripath
Correctly handle moved files in apply patch (go-gitea#22118)
9748fd0 
Backport go-gitea#22118

Moved files in a patch will result in git apply returning:

```
error: {filename}: No such file or directory
```

This wasn't handled by the git apply patch code. This PR adds handling
for this.

Fix go-gitea#22083

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this issue Dec 14, 2022
Merged
zeripath added a commit to zeripath/gitea that referenced this issue Dec 14, 2022
@zeripath
Correctly handle moved files in apply patch (go-gitea#22118)
2e5a983 
Backport go-gitea#22118

Moved files in a patch will result in git apply returning:

```
error: {filename}: No such file or directory
```

This wasn't handled by the git apply patch code. This PR adds handling
for this.

Fix go-gitea#22083

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this issue Dec 14, 2022
Merged
lunny pushed a commit that referenced this issue Dec 15, 2022
@zeripath @KN4CK3R
Correctly handle moved files in apply patch (#22118) (#22135)
dd2343d 
Backport #22118

Moved files in a patch will result in git apply returning:

```
error: {filename}: No such file or directory
```

This wasn't handled by the git apply patch code. This PR adds handling
for this.

Fix #22083

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
techknowlogick pushed a commit that referenced this issue Dec 15, 2022
@zeripath @KN4CK3R
Correctly handle moved files in apply patch (#22118) (#22136)
721e422 
Backport #22118

Moved files in a patch will result in git apply returning:

```
error: {filename}: No such file or directory
```

This wasn't handled by the git apply patch code. This PR adds handling
for this.

Fix #22083

Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
lunny added a commit that referenced this issue Dec 19, 2022
@zeripath @lunny @KN4CK3R
Add setting to disable the git apply step in test patch (#22130)
d6b9662 
For a long time Gitea has tested PR patches using a git apply --check
method, and in fact prior to the introduction of a read-tree assisted
three-way merge in #18004, this was the only way of checking patches.

Since #18004, the git apply --check method has been a fallback method,
only used when the read-tree three-way merge method has detected a
conflict. The read-tree assisted three-way merge method is much faster
and less resource intensive method of detecting conflicts. #18004 kept
the git apply method around because it was thought possible that this
fallback might be able to rectify conflicts that the read-tree three-way
merge detected. I am not certain if this could ever be the case.

Given the uncertainty here and the now relative stability of the
read-tree method - this PR makes using this fallback optional and
disables it by default. The hope is that users will not notice any
significant difference in conflict detection and we will be able to
remove the git apply fallback in future, and/or improve the read-tree
three-way merge method to catch any conflicts that git apply method
might have been able to fix.

An additional benefit is that patch checking should be significantly
less resource intensive and much quicker.

(See
https://github.com/go-gitea/gitea/issues/22083\#issuecomment-1347961737)

Ref #22083

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
zeripath added a commit to zeripath/gitea that referenced this issue Dec 19, 2022
@zeripath
Add setting to disable the git apply step in test patch (go-gitea#22130)
8d2d8b5 
Backport go-gitea#22130

For a long time Gitea has tested PR patches using a git apply --check
method, and in fact prior to the introduction of a read-tree assisted
three-way merge in go-gitea#18004, this was the only way of checking patches.

Since go-gitea#18004, the git apply --check method has been a fallback method,
only used when the read-tree three-way merge method has detected a
conflict. The read-tree assisted three-way merge method is much faster
and less resource intensive method of detecting conflicts. go-gitea#18004 kept
the git apply method around because it was thought possible that this
fallback might be able to rectify conflicts that the read-tree three-way
merge detected. I am not certain if this could ever be the case.

Given the uncertainty here and the now relative stability of the
read-tree method - this PR makes using this fallback optional but
enables it by default. A `log.Critical` has been added which will alert
if the `git apply --check` method was successful at checking a PR that
`read-tree` failed on.

The hope is that none of these log.Critical messages will be found and
there will be no significant difference in conflict detection. Thus we
will be able to remove the git apply fallback in future, and/or improve
the read-tree three-way merge method to catch any conflicts that git
apply method might have been able to fix.

An additional benefit for anyone who disables the check method is that
patch checking should be significantly less resource intensive and much
quicker.

(See
https://github.com/go-gitea/gitea/issues/22083\#issuecomment-1347961737)

Ref go-gitea#22083

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this issue Dec 19, 2022
Merged
KN4CK3R added a commit that referenced this issue Dec 22, 2022
@zeripath @KN4CK3R
Add setting to disable the git apply step in test patch (#22130) (#22170
198342e 
)

Backport #22130

For a long time Gitea has tested PR patches using a git apply --check
method, and in fact prior to the introduction of a read-tree assisted
three-way merge in #18004, this was the only way of checking patches.

Since #18004, the git apply --check method has been a fallback method,
only used when the read-tree three-way merge method has detected a
conflict. The read-tree assisted three-way merge method is much faster
and less resource intensive method of detecting conflicts. #18004 kept
the git apply method around because it was thought possible that this
fallback might be able to rectify conflicts that the read-tree three-way
merge detected. I am not certain if this could ever be the case.

Given the uncertainty here and the now relative stability of the
read-tree method - this PR makes using this fallback optional but
enables it by default. A `log.Critical` has been added which will alert
if the `git apply --check` method was successful at checking a PR that
`read-tree` failed on.

The hope is that none of these log.Critical messages will be found and
there will be no significant difference in conflict detection. Thus we
will be able to remove the git apply fallback in future, and/or improve
the read-tree three-way merge method to catch any conflicts that git
apply method might have been able to fix.

An additional benefit for anyone who disables the check method is that
patch checking should be significantly less resource intensive and much
quicker.

(See
https://github.com/go-gitea/gitea/issues/22083\#issuecomment-1347961737)

Ref #22083

Signed-off-by: Andrew Thornton <art27@cantab.net>

<!--

Please check the following:

1. Make sure you are targeting the `main` branch, pull requests on
release branches are only allowed for bug fixes.
2. Read contributing guidelines:
https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md
3. Describe what your pull request does and which issue you're targeting
(if any)

-->

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
type/bug
Projects
None yet
Milestone
1.17.4
Development

Successfully merging a pull request may close this issue.

Correctly handle moved files in apply patch zeripath/gitea
3 participants
@lunny @parnic @zeripath